-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
x/ibc: stateful clients #6202
x/ibc: stateful clients #6202
Conversation
👋 Thanks for creating a PR! Before we can merge this PR, please make sure that all the following items have been For contributor use:
For admin use:
Thank you for your contribution to the Cosmos-SDK! 🚀 |
@@ -25,6 +25,7 @@ type ClientState interface { | |||
// State verification functions | |||
|
|||
VerifyClientConsensusState( | |||
store sdk.KVStore, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replaced sdk.Context
with KVStore
since the client won't be able to access the store thought the context unless we passed the IBC StoreKey as an argument.
This approach is more secure because we can just pass the prefix store for the specific client.
Codecov Report
@@ Coverage Diff @@
## master #6202 +/- ##
==========================================
+ Coverage 54.83% 54.84% +0.01%
==========================================
Files 444 444
Lines 26784 26777 -7
==========================================
- Hits 14686 14685 -1
+ Misses 11053 11048 -5
+ Partials 1045 1044 -1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was too slow to review, but LGTM 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little late to the party, but LGTM
ctx.ChainID(), | ||
ctx.BlockHeight(), | ||
) | ||
clientState = localhosttypes.NewClientState(ctx.ChainID(), ctx.BlockHeight()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this looks a lot cleaner 🙌 I think it'll be pretty straight forward to create interfaces funcs for clients to implement so we can remove custom client handling here (see #6064)
@@ -24,6 +26,7 @@ const ( | |||
type TendermintTestSuite struct { | |||
suite.Suite | |||
|
|||
ctx sdk.Context |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what was the purpose of adding this?
Description
closes: #6198
cc: @colin-axner